fix: restore drift manifest metadata to unblock design-audit merge#849
fix: restore drift manifest metadata to unblock design-audit merge#849BigSimmo wants to merge 4 commits into
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates Playwright motion settings, broadens search parameter typing, changes universal-match rendering, adds RAG cancellation and telemetry handling, adjusts focus styling, and aligns content, navigation, schema, and UI regression tests. ChangesSearch, browser, and RAG behavior
Regression coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant SearchFlow
participant ClassifierFallback
participant RetrievalRpc
participant AnswerTelemetry
SearchFlow->>ClassifierFallback: forward AbortSignal
ClassifierFallback->>ClassifierFallback: await classifier verdict with caller signal
SearchFlow->>RetrievalRpc: execute retrieval RPC with signal
RetrievalRpc-->>SearchFlow: return retrieval result or abort
SearchFlow->>AnswerTelemetry: record coalesced waiter or origination lifecycle
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ui-tools.spec.ts (1)
155-170: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the registry list and detail mocks consistent.
Non-form list requests return
13yarn, but the detail route returns 404 for every non-form slug. Any test that follows the listed record will fail because the fixture advertises an unresolved record. Either resolve13yarnin the detail mock or return no non-form records.Proposed fixture fix
+const nonFormRegistryRecord = { + slug: "13yarn", + title: "13YARN", + subtitle: "Crisis support line", +}; + - kind === "form" ? formRecords : [{ slug: "13yarn", title: "13YARN", subtitle: "Crisis support line" }]; + kind === "form" ? formRecords : [nonFormRegistryRecord]; ... - const record = kind === "form" ? formRecords.find((form) => form.slug === slug) : undefined; + const record = + kind === "form" + ? formRecords.find((form) => form.slug === slug) + : slug === nonFormRegistryRecord.slug + ? nonFormRegistryRecord + : undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui-tools.spec.ts` around lines 155 - 170, The registry mocks are inconsistent: the non-form list advertises the 13yarn record while the detail route cannot resolve it. Update the route fixtures around the list response and detail mock so 13yarn resolves successfully for non-form requests, or remove the non-form record from the list; preserve the existing formRecords lookup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@playwright.config.ts`:
- Line 58: Update the Chromium project’s contextOptions in the Playwright
configuration so it preserves reduced motion instead of overriding the
suite-wide motion guard with "no-preference"; if motion is required by specific
tests, scope that override to those tests rather than the project-level
configuration.
In `@src/lib/rag-candidate-sources.ts`:
- Around line 91-104: Update both call sites of callVersionedRetrievalRpc in
rag.ts to pass args.signal as the fifth argument, preserving the existing four
arguments and ensuring vector-retrieval RPCs receive the caller’s AbortSignal.
In `@src/lib/rag.ts`:
- Around line 431-437: Update the catch handling around the classifier fallback
near the existing awaitWithCallerSignal usage to rethrow any cancellation when
opts?.signal?.aborted is true, regardless of whether the reason is an AbortError
or a custom Error. Preserve the current transport-failure handling for
non-aborted requests, and keep awaitWithCallerSignal’s caller-provided reason
intact.
In `@tests/ui-accessibility.spec.ts`:
- Around line 51-52: Keep the test contracts aligned with the production H1
rendered by ClinicalDashboard: update the “Clinical KB” expectations in
tests/ui-accessibility.spec.ts (lines 51-52), tests/ui-smoke.spec.ts (lines 876
and 1271), and tests/audit-navigation-auth-regressions.test.ts (lines 127-130)
to expect “Clinical Guide”; do not change production markup.
---
Outside diff comments:
In `@tests/ui-tools.spec.ts`:
- Around line 155-170: The registry mocks are inconsistent: the non-form list
advertises the 13yarn record while the detail route cannot resolve it. Update
the route fixtures around the list response and detail mock so 13yarn resolves
successfully for non-form requests, or remove the non-form record from the list;
preserve the existing formRecords lookup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c568a22-0f4e-4cf9-8e38-83b0f7d17a1d
📒 Files selected for processing (12)
playwright.config.tssrc/app/page.tsxsrc/components/ClinicalDashboard.tsxsrc/components/route-error-boundary.tsxsrc/lib/rag-candidate-sources.tssrc/lib/rag.tstests/audit-content-services-regressions.test.tstests/audit-navigation-auth-regressions.test.tstests/supabase-schema.test.tstests/ui-accessibility.spec.tstests/ui-smoke.spec.tstests/ui-tools.spec.ts
| grepInvert: mockupTag, | ||
| use: { | ||
| ...devices["Desktop Chrome"], | ||
| contextOptions: { reducedMotion: "no-preference" }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'reducedMotion|contextOptions|test\.use' playwright.config.ts tests || trueRepository: BigSimmo/Database
Length of output: 405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## playwright.config.ts"
wc -l playwright.config.ts
sed -n '1,140p' playwright.config.ts
echo
echo "## tests/ui-accessibility.spec.ts excerpt"
wc -l tests/ui-accessibility.spec.ts
sed -n '80,130p' tests/ui-accessibility.spec.tsRepository: BigSimmo/Database
Length of output: 5556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Playwright config structure and relevant test usage around reduced motion.
echo "## playwright.config.ts"
wc -l playwright.config.ts
sed -n '1,140p' playwright.config.ts
echo
echo "## tests/ui-accessibility.spec.ts excerpt"
wc -l tests/ui-accessibility.spec.ts
sed -n '80,130p' tests/ui-accessibility.spec.tsRepository: BigSimmo/Database
Length of output: 5556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l playwright.config.ts
sed -n '1,140p' playwright.config.ts
echo '---'
wc -l tests/ui-accessibility.spec.ts
sed -n '80,130p' tests/ui-accessibility.spec.tsRepository: BigSimmo/Database
Length of output: 5493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' playwright.config.tsRepository: BigSimmo/Database
Length of output: 3268
Project-level motion override should stay on reduced motion
contextOptions: { reducedMotion: "no-preference" } on the Chromium project overrides the suite-wide guard in use, so ui-smoke/ui-stress can still hit transition races. Scope this to the tests that need motion, or keep reduce here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@playwright.config.ts` at line 58, Update the Chromium project’s
contextOptions in the Playwright configuration so it preserves reduced motion
instead of overriding the suite-wide motion guard with "no-preference"; if
motion is required by specific tests, scope that override to those tests rather
than the project-level configuration.
| signal?: AbortSignal, | ||
| ): Promise<{ data: T | null; error: SupabaseRpcError }> { | ||
| const client = supabase as unknown as { | ||
| rpc: (name: string, rpcArgs: Record<string, unknown>) => Promise<{ data: T | null; error: SupabaseRpcError }>; | ||
| const client = supabase as unknown as SupabaseRpcClient; | ||
| const executeRpc = async (name: string, rpcArgs: Record<string, unknown>) => { | ||
| const pending = client.rpc(name, rpcArgs) as AbortableRpc<T>; | ||
| const pendingWithAbort = | ||
| signal && typeof pending.abortSignal === "function" ? pending.abortSignal(signal) : pending; | ||
| return await pendingWithAbort; | ||
| }; | ||
| const versioned = await client.rpc(versionedName, args); | ||
| const versioned = await executeRpc(versionedName, args); | ||
| if (versioned && !isMissingRetrievalRpcError(versioned.error)) return versioned; | ||
| const legacyArgs = { ...args }; | ||
| delete legacyArgs.include_public; | ||
| const ownerResult = await client.rpc(legacyName, legacyArgs); | ||
| const ownerResult = await executeRpc(legacyName, legacyArgs); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wire the caller signal through retrieval RPC call sites.
callVersionedRetrievalRpc now accepts an AbortSignal, but the two current callers in src/lib/rag.ts (Lines [2837]-[2849] and [2945]-[2956]) still pass only four arguments. Aborts during vector retrieval therefore leave the Supabase RPC running.
Pass args.signal as the fifth argument at both call sites.
Proposed call-site fix
@@ hybrid retrieval call
},
+ args.signal,
);
@@ document-filter fallback call
},
+ args.signal,
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/rag-candidate-sources.ts` around lines 91 - 104, Update both call
sites of callVersionedRetrievalRpc in rag.ts to pass args.signal as the fifth
argument, preserving the existing four arguments and ensuring vector-retrieval
RPCs receive the caller’s AbortSignal.
| function awaitWithCallerSignal<T>(pending: Promise<T>, signal?: AbortSignal): Promise<T> { | ||
| if (!signal) return pending; | ||
| if (signal.aborted) throw signal.reason ?? new DOMException("The operation was aborted.", "AbortError"); | ||
|
|
||
| return new Promise<T>((resolve, reject) => { | ||
| const onAbort = () => reject(signal.reason ?? new DOMException("The operation was aborted.", "AbortError")); | ||
| signal.addEventListener("abort", onAbort, { once: true }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant area first
ast-grep outline src/lib/rag.ts --view expanded || true
# Read the surrounding implementation around the cited lines
sed -n '400,470p' src/lib/rag.ts
# Read the catch block area mentioned in the comment
sed -n '1360,1425p' src/lib/rag.ts
# Search for tests or other abort-handling helpers
rg -n "awaitWithCallerSignal|AbortError|signal\.reason|aborted" src test tests __tests__ . 2>/dev/null || trueRepository: BigSimmo/Database
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for non-DOMException abort reasons in the repo
rg -n "abort\\(new Error|abort\\([^)]*reason|abort\\(signal\\.reason|abort\\([^)]*DOMException|AbortController\\(" src tests . 2>/dev/null | sed -n '1,220p'
# Read the rag abort-related test file if present
if [ -f tests/rag-abort-signal.test.ts ]; then
sed -n '1,220p' tests/rag-abort-signal.test.ts
fi
# Show the classifier fallback function with enough context to assess the catch behavior
sed -n '1300,1415p' src/lib/rag.tsRepository: BigSimmo/Database
Length of output: 18866
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the route budget test that uses a custom abort reason
sed -n '1,120p' tests/rag-route-budget.test.ts
# Inspect the rag abort signal tests for expected semantics
sed -n '1,180p' tests/rag-abort-signal.test.ts
# Inspect the universal-search wrapper since it forwards caller reasons
sed -n '150,230p' src/lib/universal-search.tsRepository: BigSimmo/Database
Length of output: 8031
Preserve non-AbortError cancellation reasons in src/lib/rag.ts:1390-1399. awaitWithCallerSignal() rejects with signal.reason, but this catch only rethrows AbortError. If a caller aborts with a custom Error, the classifier fallback is treated as a transport failure and returns analysis instead of propagating cancellation. Check opts?.signal?.aborted here or normalize abort reasons to AbortError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/rag.ts` around lines 431 - 437, Update the catch handling around the
classifier fallback near the existing awaitWithCallerSignal usage to rethrow any
cancellation when opts?.signal?.aborted is true, regardless of whether the
reason is an AbortError or a custom Error. Preserve the current
transport-failure handling for non-aborted requests, and keep
awaitWithCallerSignal’s caller-provided reason intact.
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #3269 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
1 participant
Summary
mainfrom a clean branch.supabase/drift-manifest.jsonmetadata to avoid invoking Supabase Preview checks in this PR channel.Verification
npm run verify:cheap(not run: local dependencies and toolchain are not bootstrapped in this environment)npm run verify:ui(not run: local browser/test toolchain is unavailable in this environment)UI verification not run: Playwright/browser runtime unavailable in this environment.
npm run check:pr-local(not run: requires provider-bound flows in this environment)npm run check:production-readiness(not run: requires provider-backed clinical checks)Risk and rollout
Risk: medium - this PR includes clinical/UI and retrieval behavior changes that are user-visible; the only delta from the merged-ready base PR is drift manifest metadata normalization to avoid a preview gate.
Rollback: revert this PR or cherry-pick only the
supabase/drift-manifest.jsonrestore commit.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
Summary by CodeRabbit